Skip to content

[SYCL] Clear usesAssert required by fallback assert #20571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 9 commits into
base: sycl
Choose a base branch
from
Draft

[SYCL] Clear usesAssert required by fallback assert #20571

wants to merge 9 commits into from
+9 −1,057

Conversation

@jinge90
Copy link
Contributor

@jinge90 jinge90 commented Nov 6, 2025 •
edited
Loading

The 'usesAssert' seems to be used by fallback assert mechanism which has beem discarded, create this PR to trigger pre-ci to see if any problem pops up if we remove the code snippet.

@jinge90 jinge90 requested a review from a team as a code owner November 6, 2025 04:57
@jinge90 jinge90 requested a review from slawekptak November 6, 2025 04:57
@jinge90 jinge90 marked this pull request as draft November 6, 2025 04:59
@jinge90
Copy link
Contributor Author

jinge90 commented Nov 10, 2025

Hi, @steffenlarsen
Could you take a look at this PR to see whether the 'usesAssert' metadata is used by fallback assert mechanism? If so, we can remove it.
If you are OK to remove it, I will proceed with this PR to get rid of 'usesAssert' in other components(program manager, sycl-post-link, device kernel info...)

Thanks very much.

@steffenlarsen
Copy link
Contributor

steffenlarsen commented Nov 10, 2025

Hi, @steffenlarsen Could you take a look at this PR to see whether the 'usesAssert' metadata is used by fallback assert mechanism? If so, we can remove it. If you are OK to remove it, I will proceed with this PR to get rid of 'usesAssert' in other components(program manager, sycl-post-link, device kernel info...)

Thanks very much.

@jinge90 - Thank you for testing this out! I expect it to be fine. If everything goes green, please feel free to remove the metadata. 🚀

@jinge90
remove unrelated files
e625873 
Signed-off-by: jinge90 <[email protected]>
jinge90
jinge90 commented Nov 11, 2025
View reviewed changes
sycl/source/detail/program_manager/program_manager.cpp

template <typename MultimapT, typename KeyT, typename ValT>
void removeFromMultimapByVal(MultimapT &Map, const KeyT &Key, const ValT &Val,
bool AssertContains = true) {
Copy link
Contributor Author

@jinge90 jinge90 Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @sergey-semenov
We are working on removing all fallback assert related code in sycl-post-link and sycl runtime.
One thing to do is to clean up SYCL_ASSERT_USED meta data, I found you previous patch (https://github.com/intel/llvm/pull/19659/files#diff-78dd7f7ba0b6120dece1ae4ab5a09c9936ff654a1de2c31ff2dbb1fc58d90393R2123) added 'AssertContains' param in removeFromMultimapByVal function, could you help check whether we can remove it if we are going to remove assert metadata in sycl runtime? If "AssertContains" is only used for 'assert', we can remove it, otherwise I suggest to rename the param to somthing like "ExpSymbolContains".
Thanks very much.

@jinge90
Clean unittest
c2ef77b 
Signed-off-by: jinge90 <[email protected]>
@jinge90
Remove unneeded tests
19b376f 
Signed-off-by: jinge90 <[email protected]>
@steffenlarsen
Copy link
Contributor

steffenlarsen commented Nov 11, 2025

Could you please also remove https://github.com/intel/llvm/blob/sycl/sycl/doc/design/PropertySets.md#syclassert-used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slawekptak slawekptak Awaiting requested review from slawekptak slawekptak is a code owner automatically assigned from intel/llvm-reviewers-runtime

@steffenlarsen steffenlarsen Awaiting requested review from steffenlarsen

@sergey-semenov sergey-semenov Awaiting requested review from sergey-semenov

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jinge90 @steffenlarsen